-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Bind self-types in checkmember for decorated classmethods #19025
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Bind self-types in checkmember for decorated classmethods #19025
Conversation
Diff from mypy_primer, showing the effect of this PR on open source code: steam.py (https://github.com/Gobot1234/steam.py)
+ steam/enums.py:322: error: Incompatible return value type (got "int", expected "Self") [return-value]
- steam/state.py:2439: error: Argument 2 to "__get__" of "classproperty" has incompatible type "type[Awardable[int]]"; expected "type[Self]" [arg-type]
+ steam/state.py:2439: error: Argument 2 to "__get__" of "classproperty" has incompatible type "type[Awardable[int]]"; expected "type[type[Awardable[int]]]" [arg-type]
|
Okay, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a question but the results of this make sense. It's annoying to have type vars floating around!
t, isuper, is_classmethod, is_staticmethod, mx.self_type, original_vars=original_vars | ||
) | ||
if is_decorated and not is_staticmethod: | ||
t = expand_self_type_if_needed( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand why we need to put this self type expansion here. Specifically: this branch is only for decorators. Shouldn't whatever is applying the decorators handle self types instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function handles lookup on classes, we bind self types here for everything that can reasonably contain them as this is the latest moment possible (see Var
branch above). Consider a decorator that preserves function signature applied to a def method(self) -> Self
. Applying the decorator still yields a Self
-returning callable, which is the best representation until we actually want to show it or apply it to something, when Self
loses its semantic meaning. Otherwise, for example, that method will be impossible to cleanly override in subclasses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright that makes sense then. I didn't realize when this function was called. (fyi I'm unable to resolve things, so you'll have to.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not completely sure this is the right place, but this works and I'm not sure where else would be better.
t, isuper, is_classmethod, is_staticmethod, mx.self_type, original_vars=original_vars | ||
) | ||
if is_decorated and not is_staticmethod: | ||
t = expand_self_type_if_needed( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright that makes sense then. I didn't realize when this function was called. (fyi I'm unable to resolve things, so you'll have to.)
Fixes #19023. Fixes #18993.